Only add an explicit dependency on an existing resource when the deployments engine will use the GET response #15693
+217
−260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeskew
added a commit
that referenced
this pull request
Dec 3, 2024
#15580) Resolves #15513 ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/15580) Bicep will normally generate an explicit dependency when one resource refers to another. For example, if the body of `b` includes a symbolic reference to `a`, then in the compiled JSON template, the declaration for `b` will have a `dependsOn` property that includes `a`. However, if `a` is an `existing` resource and the template is not being compiled to language version 2.0, then the compiler will "skip over" `a` and have `b` depend on whatever `a` depends on. For example, for the following template: ```bicep resource a 'type@version' existing = { name: 'a' dependsOn: [ c ] } resource b 'type@version' = { name: 'b' properties: { foo: a.properties.bar } } resource c 'type@version' = { name: 'c' } ``` the non-symbolic name output will have `b` depend on `c`. #15447 added a couple of scenarios in which Bicep would skip over an existing resource even if compiling with symbolic name support. This was done because the ARM backend will perform a `GET` request on any `existing` resource in the template _unless_ its body properties are never read and no deployed resource explicitly depends on it. The extra `GET` requests could sometimes cause template deployments to fail, for example if the deploying principal had permission to use secrets from a key vault as part of a deployment but did not have more generic /read permissions on the vault. #15447 reused some existing logic for skipping over an intermediate existing dependency that unfortunately had an underlying bug that manifested when the skipped over resource was looped and used its loop iterator to index into the dependency once removed. For example, if we modify the earlier example slightly: ```bicep resource a 'type@version' existing = [for i in range(0, 10): { name: 'a${i}' dependsOn: [ c[i] ] }] resource b 'type@version' = { name: 'b' properties: { foo: [for i in range(0, 10): a[i].properties.bar] } } resource c 'type@version' = [for i in range(0, 10): { name: 'c${i}' }] ``` Then in the compiled output, `b` will have an explicit dependency on `[resourceId('type', format('c[{0}]', copyIndex()))]`. Because `b` is not looped, the deployment will fail. Related issues will occur if `b` indexes into `a` with a more complex expression or if there is an intervening variable. This PR updates explicit dependency generation to take all steps between a depending resource and its dependency into account when generating index expressions. For example, in the following template: ```bicep resource vnets 'Microsoft.Network/virtualNetworks@2024-03-01' = [for i in range(0, 2): { name: 'vnet${i}' }] resource subnets 'Microsoft.Network/virtualNetworks/subnets@2024-03-01' existing = [for j in range(0, 10): { parent: vnets[j % 2] name: 'subnet' }] resource vault 'Microsoft.KeyVault/vaults@2023-07-01' = [for k in range(11, 10): { name: 'vault${k}' location: resourceGroup().location properties: { sku: { name: 'standard' family: 'A' } tenantId: subscription().tenantId networkAcls: { virtualNetworkRules: [{ id: subnetIds[k - 11] }] } } }] var subnetIds = [for l in range(20, 10): subnets[l - 20].id] ``` `vault` will depend on `vnets[(range(20, 10)[k - 11] - 20) % 2]`. Prior to this PR, `vault` will instead depend on `vnets[k % 2]`, which is the wrong vnet. This PR does **not** reapply the change from #15447 but only addresses the issue described above. #15447 is reapplied in #15693
This reverts commit 851f527.
|
Test this change out locally with the following install scripts (Action run 12163008935) VSCode
Azure CLI
|
Dotnet Test Results 78 files - 39 78 suites - 39 30m 29s ⏱️ - 14m 30s Results for commit bbf3b71. ± Comparison against base commit 8c7496c. This pull request removes 1846 and adds 632 tests. Note that renamed tests count towards both. |
majastrz
reviewed
Dec 4, 2024
| @@ -155,6 +161,65 @@ public override void VisitVariableAccessSyntax(VariableAccessSyntax syntax) | |||
| } | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Determines whether a reference to a resource is weak. | |||
majastrz
reviewed
Dec 4, 2024
|
|
||
| private bool IsWithinScopeDeclaration(SyntaxBase syntax) | ||
| => TryGetCurrentDeclarationTopLevelProperty(LanguageConstants.ResourceScopePropertyName) is { } nonNull && | ||
| model.Binder.IsDescendant(syntax, nonNull); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #13674
Resolves #15686
This PR reapplies the change from #15447 now that the bug in indexing expression traversal is fixed.
Microsoft Reviewers: Open in CodeFlow